Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed set mtu for deleted subintf due to late notification #2571

Merged
merged 3 commits into from
Dec 28, 2022
Merged

Fixed set mtu for deleted subintf due to late notification #2571

merged 3 commits into from
Dec 28, 2022

Conversation

EdenGri
Copy link
Contributor

@EdenGri EdenGri commented Dec 13, 2022 •
edited
Loading

What I did

Ignores errors on the set MTU command for subinterface when the subinterface state is not OK.

Why I did it

A race condition between the portmgrd and the intfmgrd sometimes causes running a set MTU command on a deleted subinterface.
The logs and the error:

INFO swss#supervisord: intfmgrd Cannot find device "Ethernet32.58"
ERR swss#intfmgrd: :- main: Runtime error: /sbin/ip link set "Ethernet32.58" mtu "9100" :
INFO swss#supervisord 2022-11-08 05:53:33,057 INFO exited: intfmgrd (exit status 255; not expected)

How I verified it

Run the test_loopback_action_reload test and saw no errors in the logs.

Details if related

@EdenGri EdenGri requested a review from prsunny as a code owner December 13, 2022 20:00
dprital
dprital previously approved these changes Dec 14, 2022
View reviewed changes
@bingwang-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 18, 2022

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2571 in repo sonic-net/sonic-swss

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 18, 2022

/azpw run Asure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Asure.sonic-buildimage

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 18, 2022

/azpw run Azure.sonic-swss

@EdenGri EdenGri closed this Dec 18, 2022
@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@EdenGri EdenGri reopened this Dec 18, 2022
@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 18, 2022

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 19, 2022

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

prsunny
prsunny requested changes Dec 19, 2022
View reviewed changes
cfgmgr/intfmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/intfmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/intfmgr.cpp

return subifMtu;
if (ret && !isIntfStateOk(alias))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already checked in the doIntfGeneralTask under SET operation. So why check again here? is there a race condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not visiting the "doIntfGeneralTask" function in this flow.
The flow is:

  1. changing the "mtu" field in the PORT_TABLE in state DB
  2. doTask
  3. doPortTableTask
  4. updateSubIntfMtu
  5. setHostSubIntfMtu

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, does this happen in the updateSubIntfAdminStatus flow as well? Or why only mtu change is handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is happen in the updateSubIntfAdminStatus flow as well

@bingwang-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 21, 2022

/azpw run Asure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Asure.sonic-buildimage

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 21, 2022

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 26, 2022

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EdenGri
Copy link
Contributor Author

EdenGri commented Dec 26, 2022

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dprital
Copy link
Collaborator

dprital commented Dec 26, 2022

@prsunny - Can you please approve this PR ?

@prsunny prsunny merged commit 7891e78 into sonic-net:master Dec 28, 2022
@dprital dprital mentioned this pull request Jan 1, 2023
Merged
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jan 1, 2023
@dprital
[submodule] Advance sonic-swss pointer
ea8bd29 
Update sonic-swss submodule pointer to include the following:
* bdedf69 Modify coppmgr mergeConfig to support preserving copp tables through reboot. ([sonic-net#2548](sonic-net/sonic-swss#2548))
* 7891e78 Fixed set mtu for deleted subintf due to late notification ([sonic-net#2571](sonic-net/sonic-swss#2571))
* a443945 Updated handling of VRF_VNI mapping and VLAN_VNI mapping for same VNI ID ([sonic-net#2538](sonic-net/sonic-swss#2538))

Signed-off-by: dprital <[email protected]>
@dprital dprital mentioned this pull request Jan 1, 2023
Merged
8 tasks
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 2, 2023
@dprital
[submodule] Advance sonic-swss pointer (#13223)
aeaec40 
Update sonic-swss submodule pointer to include the following:

* bdedf69 Modify coppmgr mergeConfig to support preserving copp tables through reboot. ([#2548](sonic-net/sonic-swss#2548))
* 7891e78 Fixed set mtu for deleted subintf due to late notification ([#2571](sonic-net/sonic-swss#2571))
* a443945 Updated handling of VRF_VNI mapping and VLAN_VNI mapping for same VNI ID ([#2538](sonic-net/sonic-swss#2538))

Signed-off-by: dprital <[email protected]>
liat-grozovik pushed a commit that referenced this pull request Jan 3, 2023
@dprital
[202205] Fixed set mtu for deleted subintf due to late notification (#…
8c9092c 
…2595)

PR against 202205 based on the following PR: #2571

- What I did
Ignores errors on the set MTU command for subinterface when the subinterface state is not OK.

- Why I did it
A race condition between the portmgrd and the intfmgrd sometimes causes running a set MTU command on a deleted subinterface.
The logs and the error:

INFO swss#supervisord: intfmgrd Cannot find device "Ethernet32.58"
ERR swss#intfmgrd: :- main: Runtime error: /sbin/ip link set "Ethernet32.58" mtu "9100" :
INFO swss#supervisord 2022-11-08 05:53:33,057 INFO exited: intfmgrd (exit status 255; not expected)

- How I verified it
Run the test_loopback_action_reload test and saw no errors in the logs.
StormLiangMS pushed a commit that referenced this pull request Feb 10, 2023
@EdenGri @StormLiangMS
Fixed set mtu for deleted subintf due to late notification (#2571)
ae68054 
* Fixed set mtu for deleted subintf due to late notification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dprital dprital dprital approved these changes

@prsunny prsunny prsunny approved these changes

Assignees
No one assigned
Labels
Included in 202205 Branch Included in 202211 Branch Request for 202211 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@EdenGri @bingwang-ms @mssonicbld @dprital @prsunny @liat-grozovik @StormLiangMS